Implement v2 RecvPacket rpc handler #7421

chatton · 2024-10-09T11:22:16Z

Description

closes: #7354

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

…nt-on-channelkeeperv2' into cian/issue#7354-implement-v2-recvpacket-rpc-handler

chatton · 2024-10-10T12:21:04Z

modules/core/04-channel/v2/keeper/packet.go

+ return errorsmod.Wrap(channeltypes.ErrInvalidPacket, "receipt not found for packet")
+ }
+
+ multiAckBz := k.cdc.MustMarshal(&ack)


rename to ackBz

modules/core/04-channel/v2/keeper/msg_server.go

bznein · 2024-10-10T12:27:57Z

modules/core/04-channel/v2/keeper/msg_server_test.go

+ suite.Require().Contains(err.Error(), tc.expError.Error())
+
+ _, ok := ck.GetPacketReceipt(path.EndpointB.Chain.GetContext(), recvPacket.SourceChannel, recvPacket.Sequence)
+ suite.Require().False(ok)
+ ibctesting.RequireErrorIsOrContains(suite.T(), err, tc.expError)


Shouldn't line 261 be at 257 (and the current 257 can be removed?)

merge conflict mistake I think, will fix!

modules/core/04-channel/v2/keeper/packet.go

bznein

LGTM

modules/core/04-channel/v2/keeper/msg_server.go

sonarcloud · 2024-10-14T09:56:11Z

Quality Gate passed for 'ibc-go'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

DimitrisJim

I left a couple a comments, mostly nits, lgtm overall.

DimitrisJim · 2024-10-14T10:00:21Z

modules/core/04-channel/v2/keeper/export_test.go

+)
+
+// GetPacketAcknowledgement fetches the packet acknowledgement from the store.
+func (k *Keeper) GetPacketAcknowledgement(ctx context.Context, sourceID string, sequence uint64) []byte {


I think this will eventually be needed when we add the equivalent grpc queries for v2? Lets move to keeper.go?

should be channelID

yeah probs just make exported

DimitrisJim · 2024-10-14T10:04:30Z

modules/core/04-channel/v2/keeper/msg_server.go

+ cb := k.Router.Route(pd.DestinationPort)
+ res := cb.OnRecvPacket(cacheCtx, msg.Packet.SourceChannel, msg.Packet.DestinationChannel, pd, signer)
+
+ if res.Status != channeltypesv2.PacketStatus_Failure {


would have a small preference for == here

you mean

if res.Status == channeltypesv2.PacketStatus_Async || res.Status == channeltypesv2.PacketStatus_Success

oh and flipping of handling. So if res.Status == channeltypesv2.PacketStatus_Faillure do the current else handling.

DimitrisJim · 2024-10-14T10:06:04Z

modules/core/04-channel/v2/keeper/msg_server.go

+ // note this should never happen as the packet data would have had to be empty.
+ if len(ack.AcknowledgementResults) == 0 {
+ sdkCtx.Logger().Error("receive packet failed", "source-channel", msg.Packet.SourceChannel, "error", errorsmod.Wrap(err, "invalid acknowledgement results"))
+ return &channeltypesv2.MsgRecvPacketResponse{Result: channeltypesv1.FAILURE}, nil


would this mean that when we remove v1 stuff this would be a proto-breaking change? (i.e Result type changing)

yeah, we'd need to add a V2 version I guess

I think we should just return err here

DimitrisJim · 2024-10-14T10:08:23Z

modules/core/04-channel/v2/keeper/packet.go

@@ -296,3 +296,49 @@ func (k *Keeper) timeoutPacket(

 return nil
 }
+
+// WriteAcknowledgement writes the acknowledgement to the store.


nit, lets move under recv impl?

DimitrisJim · 2024-10-14T10:09:19Z

modules/core/04-channel/v2/keeper/packet.go

+ // packet sender is our channel's counterparty channel id.
+ channel, ok := k.GetChannel(ctx, packet.DestinationChannel)
+ if !ok {
+ // TODO: figure out how aliasing will work when more than one packet data is sent.


lets create issue and potentially rm these TODOs?

DimitrisJim · 2024-10-14T10:10:01Z

modules/core/04-channel/v2/keeper/packet.go

+ // TODO: figure out how aliasing will work when more than one packet data is sent.
+ channel, ok = k.convertV1Channel(ctx, packet.Data[0].DestinationPort, packet.DestinationChannel)
+ if !ok {
+ return errorsmod.Wrap(channeltypes.ErrChannelNotFound, packet.DestinationChannel)


should use v2 types no? I.e channeltypesv2 (ditto for rest)

aliasing/converting should happen in RecvPacket so this should never be reachable...

should just be error i think if not found

DimitrisJim · 2024-10-14T10:12:26Z

modules/core/04-channel/v2/keeper/packet.go

+
+ // set the acknowledgement so that it can be verified on the other side
+ k.SetPacketAcknowledgement(
+ ctx, packet.DestinationChannel, packet.GetSequence(),


lets use direct access? I think we should just not gen the getters here at the end of the day.

Suggested change

ctx, packet.DestinationChannel, packet.GetSequence(),

ctx, packet.DestinationChannel, packet.Sequence,

I always kinda liked the getters 😢 hot take... really I don't care tho

damiannolan · 2024-10-14T10:33:26Z

modules/core/04-channel/v2/keeper/export_test.go

+)
+
+// GetPacketAcknowledgement fetches the packet acknowledgement from the store.
+func (k *Keeper) GetPacketAcknowledgement(ctx context.Context, sourceID string, sequence uint64) []byte {


should be channelID

damiannolan · 2024-10-14T10:33:43Z

modules/core/04-channel/v2/keeper/export_test.go

+)
+
+// GetPacketAcknowledgement fetches the packet acknowledgement from the store.
+func (k *Keeper) GetPacketAcknowledgement(ctx context.Context, sourceID string, sequence uint64) []byte {


yeah probs just make exported

damiannolan · 2024-10-14T10:43:15Z

modules/core/04-channel/v2/keeper/msg_server.go

+ // note this should never happen as the packet data would have had to be empty.
+ if len(ack.AcknowledgementResults) == 0 {
+ sdkCtx.Logger().Error("receive packet failed", "source-channel", msg.Packet.SourceChannel, "error", errorsmod.Wrap(err, "invalid acknowledgement results"))
+ return &channeltypesv2.MsgRecvPacketResponse{Result: channeltypesv1.FAILURE}, nil


I think we should just return err here

damiannolan · 2024-10-14T10:47:27Z

testing/utils.go

@@ -87,6 +87,7 @@ func UnmarshalMsgResponses(cdc codec.Codec, data []byte, msgs ...codec.ProtoMars
 // RequireErrorIsOrContains verifies that the passed error is either a target error or contains its error message.
 func RequireErrorIsOrContains(t *testing.T, err, targetError error, msgAndArgs ...interface{}) {
 t.Helper()
+ require.Error(t, err)


can we remove this?

damiannolan · 2024-10-14T10:48:26Z

modules/core/04-channel/v2/keeper/packet.go

+
+ // set the acknowledgement so that it can be verified on the other side
+ k.SetPacketAcknowledgement(
+ ctx, packet.DestinationChannel, packet.GetSequence(),


I always kinda liked the getters 😢 hot take... really I don't care tho

damiannolan · 2024-10-14T10:51:58Z

modules/core/04-channel/v2/keeper/packet.go

+ // TODO: figure out how aliasing will work when more than one packet data is sent.
+ channel, ok = k.convertV1Channel(ctx, packet.Data[0].DestinationPort, packet.DestinationChannel)
+ if !ok {
+ return errorsmod.Wrap(channeltypes.ErrChannelNotFound, packet.DestinationChannel)


aliasing/converting should happen in RecvPacket so this should never be reachable...

should just be error i think if not found

damiannolan · 2024-10-14T10:53:01Z

modules/core/04-channel/v2/keeper/packet.go

+ // the OnRecvPacket callback so we need to check if the acknowledgement is already
+ // set on the store and return an error if so.
+ if k.HasPacketAcknowledgement(ctx, packet.DestinationChannel, packet.Sequence) {
+ return channeltypes.ErrAcknowledgementExists


should we wrap with channel id / sequence?

damiannolan · 2024-10-14T10:53:08Z

modules/core/04-channel/v2/keeper/packet.go

+ }
+
+ if channel.CounterpartyChannelId != packet.SourceChannel {
+ return channeltypes.ErrInvalidChannelIdentifier


same for wrapping

chatton added 11 commits October 8, 2024 09:41

chore: added application callback to SendPacketV2 rpc

b7d3c74

chore: adding additional msg server tests

6761671

chore: adding additional tests

0411d52

Merge branch 'cian/issue#7369-implement-messageserverv2-acknowledgeme…

483606d

…nt-on-channelkeeperv2' into cian/issue#7354-implement-v2-recvpacket-rpc-handler

chore: adding on recv handler

b1ce0c6

chore: add recv packet happy path

33d9e6a

chore: added test for failed recv result

999c04e

chore: adding additional tests

8c0ce79

chore: merge feature branch

9dc9b67

chore: misc cleanup on self review

a8cd088

chore: moved fn into packet.go and added no-op telemetry v2 functions

dbc39b0

chatton changed the title ~~[WIP DONT REVIEW] Implement v2 RecvPacket rpc handler~~ Implement v2 RecvPacket rpc handler Oct 10, 2024

chatton marked this pull request as ready for review October 10, 2024 12:17

chatton requested review from colin-axner, AdityaSripal, damiannolan, crodriguezvega, DimitrisJim, gjermundgaraba and bznein as code owners October 10, 2024 12:17

chatton commented Oct 10, 2024

View reviewed changes

bznein reviewed Oct 10, 2024

View reviewed changes

chore: addressing PR feedback

bf2927d

chatton mentioned this pull request Oct 10, 2024

Remove duplication of convertToErrorEvents #7436

Open

4 tasks

chore: add issue links

acbfb8c

bznein approved these changes Oct 10, 2024

View reviewed changes

bznein reviewed Oct 10, 2024

View reviewed changes

modules/core/04-channel/v2/keeper/msg_server.go Show resolved Hide resolved

chatton and others added 3 commits October 11, 2024 07:27

chore: ensure no-op case works as intended

0a3a588

chore: add NoOp test case

352749e

chore: fix ack v2 commitment

3399f15

chore: merge feature brnanch

803347b

DimitrisJim approved these changes Oct 14, 2024

View reviewed changes

damiannolan reviewed Oct 14, 2024

View reviewed changes

chatton added 2 commits October 14, 2024 12:49

chore: PR feedback

bab18a5

chore: merge feature branch

c3ffec1

chatton merged commit 50ead0f into feat/ibc-eureka Oct 14, 2024
25 checks passed

chatton deleted the cian/issue#7354-implement-v2-recvpacket-rpc-handler branch October 14, 2024 11:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement v2 RecvPacket rpc handler #7421

Implement v2 RecvPacket rpc handler #7421

chatton commented Oct 9, 2024 •

edited

Loading

chatton Oct 10, 2024

bznein Oct 10, 2024

chatton Oct 10, 2024

bznein left a comment

sonarcloud bot commented Oct 14, 2024

DimitrisJim left a comment

DimitrisJim Oct 14, 2024

damiannolan Oct 14, 2024

damiannolan Oct 14, 2024

DimitrisJim Oct 14, 2024

chatton Oct 14, 2024

DimitrisJim Oct 14, 2024

DimitrisJim Oct 14, 2024

chatton Oct 14, 2024

damiannolan Oct 14, 2024

DimitrisJim Oct 14, 2024

DimitrisJim Oct 14, 2024

DimitrisJim Oct 14, 2024

damiannolan Oct 14, 2024

DimitrisJim Oct 14, 2024

damiannolan Oct 14, 2024

damiannolan Oct 14, 2024

damiannolan Oct 14, 2024

damiannolan Oct 14, 2024

damiannolan Oct 14, 2024

damiannolan Oct 14, 2024

damiannolan Oct 14, 2024

damiannolan Oct 14, 2024

damiannolan Oct 14, 2024

	ctx, packet.DestinationChannel, packet.GetSequence(),
	ctx, packet.DestinationChannel, packet.Sequence,

Implement v2 RecvPacket rpc handler #7421

Implement v2 RecvPacket rpc handler #7421

Conversation

chatton commented Oct 9, 2024 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bznein left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Oct 14, 2024

Quality Gate passed for 'ibc-go'

DimitrisJim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chatton commented Oct 9, 2024 •

edited

Loading